Repository navigation
fix(resolvers): ignored priorities no longer discard all EIDs in multi-node targeting - #382
Merged
Merged
Conversation
…i-node targeting In priority mode, rules with no priority (or priority <= 0) were stored under key 0. Key 0 sorted first and then failed the `if (priority)` check, so any ignored rule that returned EIDs caused the resolver to return no EIDs at all, even when priority 1+ rules resolved. Exclude priorities below 1 from selection and check for undefined explicitly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment on lines
125
to
129
| // Priorities below 1 are ignored and must not be selected | ||
| const priority = Array.from(eidsByPriority.keys()) | ||
| .filter((x) => x >= 1 && eidsByPriority.get(x)?.length) | ||
| .sort((a, b) => a - b) | ||
| .filter((x) => eidsByPriority.get(x)?.length) | ||
| .shift(); |
Contributor
There was a problem hiding this comment.
Comment is actually not accurate, since line 33 has:
return rules.some((x) => x.priority) ? resolvePriorityTargeting(rules) : resolveAggregateTargeting(rules);
This will make it so that 0 counts as no priority and -1 counts as priority. The new filter only ever runs if its set to priority so we never actually check the below -1 rule in that case
Contributor
There was a problem hiding this comment.
This is kind of an edge case cause I dont think we normally use negative priorities, but Im just going by unix nice conventions. Approving the PR
Contributor
Author
There was a problem hiding this comment.
fixed comment
etiennelatendresse-optable
approved these changes
Nov 10, 2026
…priority mode Mode selection uses a truthiness check, so all-0/unset rules are aggregated while a negative priority switches to priority mode. Reword both comments to say so, and fix the "threated" typo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
In priority mode,
resolveMultiNodeTargetingreturned no EIDs at all whenever a rule with nopriority(orpriority <= 0) resolved any EIDs, even if priority 1+ rules also resolved. The README's own example (priority 1, priority 2, and one unprioritized rule) hits this.Cause: ignored rules were stored under key
0(Math.max(0, priority)). Key0sorted first, was selected, and then failed theif (priority)check, so nothing was pushed.Found while auditing the README against the code.
What Changed
lib/core/resolvers/resolveMultiTargeting.ts: exclude priorities below 1 from selection, and checkpriority !== undefinedinstead of truthiness.lib/core/resolvers/resolveMultiTargeting.test.ts: regression test mixing priorities 1, 2, unset and -1; expects only the priority-1 EIDs, sources and resolved IDs.How to Test
npx jest lib/core/resolvers. The new test fails onmaster(returns[]) and passes with the fix.Full suite: 701/701 passing;
tscand prettier clean.Tested
Docs updated (N/A: behaviour now matches the documented "priorities ≤ 0 are ignored")
Notes
Ignored rules still merge their
ortb2.user.data(segments) into the result, unchanged by this PR. Worth deciding whether "ignored" should drop those too.Customer bundles that mix prioritized and unprioritized rules will start receiving EIDs they were previously losing.
Breaking change
Requires release
🤖 Generated with Claude Code